Skip to content

More thorough support for retrieving OpenAPI routes in a case-sensitive manner #59568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Jan 13, 2025
Merged

More thorough support for retrieving OpenAPI routes in a case-sensitive manner #59568

merged 11 commits into from
Jan 13, 2025
+117 −16

Conversation

@sander1095
Copy link
Contributor

@sander1095 sander1095 commented Dec 19, 2024 •
edited
Loading

More thorough support for retrieving OpenAPI routes in a case-sensitive manner

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

This PR fixes some oversights from #59199 around retrieving OpenAPI documents case-insensitively.

Description

This PR modifies missed files from #59199 that use the registered document name as a key for retrieving services or options. This needs to be done case-insensitively to:

  • Ensure documents can be retrieved correctly
    • Trying to retrieve services around OpenAPI documents that are registered with a differently cased document name, will result in an exception, which is not desired!
  • Ensure documents are generated and retrieved with the correct OpenAPI version
    • If the configuration options are retrieved with an incorrectly cased document name argument, a default instance of OpenApiOptions is returned, which might contain different settings than what the user had configured.

This PR also adds some more comments and improves existing ones based on the previous PR around case-insensitive OpenAPI document names.

Fixes #59175

@sander1095 sander1095 requested review from a team and captainsafia as code owners December 19, 2024 15:10
@ghost ghost added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Dec 19, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 19, 2024
@martincostello martincostello added feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Dec 19, 2024
captainsafia
captainsafia requested changes Dec 19, 2024
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this and following up with a PR!

This has got me thinking that we should audit other places where we pass in documentName in the same way. As a result, it looks like we'll also want to make a change in OpenApiDocumentProvider. The same test around the API version should work here.

@sander1095 sander1095 changed the title Small fix around getting options when retrieving OpenAPI routes in a case-sensitive manner More thorough support for retrieving OpenAPI routes in a case-sensitive manner Dec 20, 2024
@sander1095
Copy link
Contributor Author

sander1095 commented Dec 20, 2024

@captainsafia I've implemented your requested changes, added extra comments, improved existing ones and updated the title and description of PR to better reflect the changes.

Let me know if you want me to do anything else! :)

captainsafia
captainsafia reviewed Dec 23, 2024
View reviewed changes
@sander1095
Copy link
Contributor Author

sander1095 commented Dec 25, 2024

Thanks for the review! I'll implement requested changes after my holiday (in ~2 weeks)

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 2, 2025
@captainsafia captainsafia added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Jan 6, 2025
@sander1095
Copy link
Contributor Author

sander1095 commented Jan 9, 2025

@captainsafia I've processed the PR comments!

@captainsafia
Copy link
Member

captainsafia commented Jan 13, 2025

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 13, 2025
@azure-pipelines
Copy link

azure-pipelines bot commented Jan 13, 2025

Azure Pipelines successfully started running 3 pipeline(s).

@captainsafia captainsafia removed the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Jan 13, 2025
@captainsafia captainsafia requested a review from Copilot January 13, 2025 18:36
Copilot AI reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

@captainsafia captainsafia enabled auto-merge (squash) January 13, 2025 18:38
@captainsafia captainsafia merged commit 8f13bea into dotnet:main Jan 13, 2025
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Jan 13, 2025
captainsafia pushed a commit that referenced this pull request Feb 11, 2025
@sander1095 @captainsafia
More thorough support for retrieving OpenAPI routes in a case-sensiti…
ccd88e4 
…ve manner (#59568)

* When retrieving optione, use the lowercase document name, too

* Added tests for checking if options monitor uses case-insensitive document name for OpenAPI version retrieval

* Add XML comments to prevent bugs in the future

* Improve comments on existing test

* Add new tests for DocumentProvider which should also work case-insensitively

* Fix build by adding necessary XML comment

* Fix build of tests

* Improve comments

* Fix incorrect tests

* Fix bugs around case-sensitive document names

* Implemented PR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@captainsafia captainsafia captainsafia approved these changes

+1 more reviewer

@martincostello martincostello martincostello left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@captainsafia captainsafia

Labels

area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc community-contribution Indicates that the PR has been added by a community member feature-openapi

Projects

None yet

Milestone

10.0-preview1

Development

Successfully merging this pull request may close these issues.

OpenAPI document names are case-sensitive in urls

3 participants

@sander1095 @captainsafia @martincostello